Support `initializationOptions` to configure the server #459

tkrabel-db · 2023-10-12T20:11:17Z

What changes are proposed in this pull request?

Fixes #195.

The language server supports passing initializationOptions with the initialize message, but actually doesn't do anything with it. I think this was an oversight. This PR supports overriding configuration settings using the initializationOptions field.

How is this tested?

Added a unit test
Manually

tkrabel-db · 2023-10-12T20:14:15Z

test/test_configuration.py

+from pylsp import IS_WIN
+
+
+INITIALIZATION_OPTIONS = {


Using this as an example as this is a real world example for when to override confs

rchl · 2023-10-12T21:24:34Z

pylsp/config/config.py

@@ -98,6 +98,10 @@ def __init__(self, root_uri, init_opts, process_id, capabilities):
                self._plugin_settings, plugin_conf
            )

+        self._plugin_settings = _utils.merge_dicts(
+            self._plugin_settings, self._init_opts.get("pylsp", {})


Language servers typically don't scope initialization settings to server name because, unlike the workspace settings, those are specific to the server itself and don't have any chance of conflicting with other settings.

I am happy to address this, but it seems that there is lack of consensus on whether initializationOptions should be used for server configuration.

pylsp is the options namespace used for this server, so I'm fine with using it here.

krassowski · 2023-10-12T21:59:14Z

Some context:

as per spec initialization options are different from configuration options; however there is no agreement as to how they are different; there was an attempt to standardize it in microsoft/language-server-protocol@545cd69 but after discussion in lsp#567 it was reverted due to lack of consensus
this was previously discussed in Maybe use initializationOptions as additional source of settings #195 so it is not an omission - we should follow the discussion in there
one way that these two could be use used differently, and are used differently by some language server is following microsoft/language-server-protocol@545cd69: initialization options could include pythonVersion: string which is required during initialization but configuration options would have showWarnings: boolean which can be adjusted on runtime

tkrabel-db · 2023-10-13T10:24:26Z

@krassowski thanks for the context.

I have a clear use case in mind: enabling rope_autoimport without a config file. From the code and #195 I see that workspace/didChangeConfiguration is the supported way to achieve that, and I am happy to go with that approach.

I feel the discussion around initializationOptions is unfortunate. There is no consensus around how initializationOptionsshould be used, but many developers find it very useful to configure the server on initialize. I think so too.

As more and more language servers will run in cloud VMs and not on a local machine, these servers need to be configurable dynamically. workspace/configure is a valid general approach to handle server configuration at runtime, but using that to enable/disable some features is overkill, wasteful and will lead to bad user behaviour.

It's wasteful because you will end up turning off features that might have been enabled already on initialize, consuming resources unnecessarily (think e.g. rope_autoimport, which needs to index python modules; that problem becomes worse when the language server lives in an ephemeral environment like a cloud VM)
It will lead to bad user behaviour since the client will already send messages and potentially receive responses from features that the used didn't even want to use in the first place (e.g. getting ruff diagnostics instead of flake8 ones).
It is overkill because it will require extra logic to make sure the server is responsive while requesting workspace/configuration and while applying it (something that would be a no-brainer with initializeOptions as the server hasn't send a response to that yet, so the client keeps waiting)

tkrabel-db · 2023-10-13T11:12:58Z

Anyway, I moved my comment to #195, i.e. here

ccordoba12

Only one tiny suggestion for you @tkrabel-db.

test/test_configuration.py

tkrabel-db · 2023-10-19T08:08:24Z

@ccordoba12 thanks for supporting this feature! I addressed you comment and also refactored the unit test to use the new test util functions

ccordoba12

Looks good to me now, thanks @tkrabel-db!

tkrabel-db added 3 commits October 12, 2023 22:07

Support initializationOptions

2dad2ae

black format

274afb1

unti tests

8d70bf7

tkrabel-db commented Oct 12, 2023

View reviewed changes

tkrabel-db added 2 commits October 12, 2023 22:15

black

f7634f1

fix unit test patching workspace.update_config

173d3eb

tkrabel-db mentioned this pull request Oct 12, 2023

No suggestions from autoimport #431

Open

rchl reviewed Oct 12, 2023

View reviewed changes

tkrabel-db mentioned this pull request Oct 13, 2023

Maybe use initializationOptions as additional source of settings #195

Closed

ccordoba12 changed the title ~~Support initializationOptions~~ Support initializationOptions to configure the server Oct 19, 2023

ccordoba12 added this to the v1.9.0 milestone Oct 19, 2023

ccordoba12 added the enhancement New feature or request label Oct 19, 2023

ccordoba12 reviewed Oct 19, 2023

View reviewed changes

test/test_configuration.py Outdated Show resolved Hide resolved

tkrabel-db added 3 commits October 19, 2023 09:52

Merge branch 'develop' into bugfix-support-init-opts

13a57b8

minor fix

6a7c1a9

refactor unit tests

4e5db37

tkrabel-db requested a review from ccordoba12 October 19, 2023 08:07

ccordoba12 approved these changes Oct 20, 2023

View reviewed changes

ccordoba12 merged commit 1f415b5 into python-lsp:develop Oct 20, 2023
10 checks passed

staticf0x pushed a commit to staticf0x/python-lsp-server that referenced this pull request Oct 27, 2023

Support initializationOptions to configure the server (python-lsp#459)

ac68428

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `initializationOptions` to configure the server #459

Support `initializationOptions` to configure the server #459

tkrabel-db commented Oct 12, 2023 •

edited by ccordoba12

Loading

tkrabel-db Oct 12, 2023

rchl Oct 12, 2023

tkrabel-db Oct 13, 2023

ccordoba12 Oct 19, 2023 •

edited

Loading

krassowski commented Oct 12, 2023 •

edited

Loading

tkrabel-db commented Oct 13, 2023 •

edited

Loading

tkrabel-db commented Oct 13, 2023 •

edited

Loading

ccordoba12 left a comment

tkrabel-db commented Oct 19, 2023

ccordoba12 left a comment

Support initializationOptions to configure the server #459

Support initializationOptions to configure the server #459

Conversation

tkrabel-db commented Oct 12, 2023 • edited by ccordoba12 Loading

What changes are proposed in this pull request?

How is this tested?

tkrabel-db Oct 12, 2023

Choose a reason for hiding this comment

rchl Oct 12, 2023

Choose a reason for hiding this comment

tkrabel-db Oct 13, 2023

Choose a reason for hiding this comment

ccordoba12 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

krassowski commented Oct 12, 2023 • edited Loading

tkrabel-db commented Oct 13, 2023 • edited Loading

tkrabel-db commented Oct 13, 2023 • edited Loading

ccordoba12 left a comment

Choose a reason for hiding this comment

tkrabel-db commented Oct 19, 2023

ccordoba12 left a comment

Choose a reason for hiding this comment

Support `initializationOptions` to configure the server #459

Support `initializationOptions` to configure the server #459

tkrabel-db commented Oct 12, 2023 •

edited by ccordoba12

Loading

ccordoba12 Oct 19, 2023 •

edited

Loading

krassowski commented Oct 12, 2023 •

edited

Loading

tkrabel-db commented Oct 13, 2023 •

edited

Loading

tkrabel-db commented Oct 13, 2023 •

edited

Loading